feat: Use file input in payForBlobs CLI to allow to submit multiple blobs #2856

bao1029p · 2023-11-19T23:24:26Z

Changes made

Add blobJson proto type define how user can submit blob in file
Add file path as argument input for payForBlob command
Add parseSubmitBlobs to parse content from the file to array of defined blobJSON. Each blobJSON contain a namespaceID and blob in hex encoded format ( can modify what the user can put in the file later on, this need further conversation on this )
Modifed the broadcastPFB func to accept multiple blobs instead

Testing

test the command with single-node script running and got this result ( thanks @sontrinh16 for providing the test result )

using the test_blob.json file contain:
{ "Blobs": [ { "namespaceId": "0x00010203040506070809", "blob": "0x48656c6c6f2c20576f726c6421" }, { "namespaceId": "0x00010203040506070809", "blob": "0x48656c6c6f2c20576f726c6421" } ] }

Checklist

New and updated code has appropriate documentation
New and updated code has new and/or updated testing
Required CI checks are passing
Visual proof for any user facing features like CLI or documentation updates
Linked issues closed with keywords

coderabbitai · 2023-11-19T23:24:30Z

Note

Reviews Paused

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Tips

Chat with CodeRabbit Bot (`@coderabbitai`)

If you reply to a review comment from CodeRabbit, the bot will automatically respond.
To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
Note: Review comments are made on code diffs or files, not on the PR overview.
Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

bao1029p · 2023-11-19T23:27:52Z

@coderabbitai pause

codecov-commenter · 2023-11-19T23:29:53Z

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (9b5d025) 19.16% compared to head (25324ba) 19.16%.

Files	Patch %	Lines
cmd/celestia-appd/cmd/download_genesis.go	0.00%	5 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2856   +/-   ##
=======================================
  Coverage   19.16%   19.16%           
=======================================
  Files         142      142           
  Lines       17291    17285    -6     
=======================================
  Hits         3313     3313           
+ Misses      13676    13670    -6     
  Partials      302      302

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sontrinh16 · 2023-11-29T06:55:21Z

@bao1029p hey gud ser can i have permision to push some code to this PR

bao1029p · 2023-11-29T07:08:20Z

@bao1029p hey gud ser can i permision to push some code to this PR

yes plss feel free to contribute to this PR

Use file input

bao1029p · 2023-11-29T09:20:20Z

thanks @sontrinh16 for the help, i think we are close command work now, will do some testing

bao1029p · 2023-11-30T14:39:09Z

i think this is a rough working version for now, happy to change any logic

evan-forbes

dope left a few comments on things I think we should handle but otherwise this is a really great start. thanks for doing this @bao1029p !!

proto/celestia/core/v1/blob/blob.proto

evan-forbes · 2023-11-30T16:11:55Z

x/blob/client/cli/payforblob.go

+				shareVersion, _ := cmd.Flags().GetUint8(FlagShareVersion)
+				blob, err := types.NewBlob(namespace, rawblob, shareVersion)
+				if err != nil {
+					fmt.Printf("failure to create blob with hex blob value %s: %s", hexStr, err.Error())


[blocking]
as is would this submit a transaction without the blob that failed? if that's the case then I think we need to return with error. that could be immediatly or using something like multierror

yes agreed will update this

evan-forbes · 2023-11-30T16:20:44Z

x/blob/client/cli/payforblob.go

-			if len(args) < 2 {
-				return fmt.Errorf("PayForBlobs requires two arguments: namespaceID and blob")
+			if len(args) < 1 {
+				return fmt.Errorf("PayForBlobs requires one arguments: path to blob.json")


[blocking]
I think there's a way to avoid a breaking change, and we should pursue that. for example we could use a flag to indicate that its pointing to json file (or accepted piped input etc). we could even check if the input has the last five characters equal to ".json" and then proceed to try and parse a json

added the check, thanks for the suggestion

…-app into use_file_input

rootulp · 2023-12-04T18:21:05Z

x/blob/client/cli/payforblob.go

+	// FlagNamespaceVersion allows the user to override the namespace version when
+	// submitting a PayForBlob.


[blocking] this GoDoc isn' related to the flag it's documenting. It should look something like:

// FlagBlobsFilePath allows the user to...

updated as requested

rootulp · 2023-12-04T18:24:44Z

x/blob/client/cli/payforblob.go

+
+	// FlagNamespaceVersion allows the user to override the namespace version when
+	// submitting a PayForBlob.
+	FlagBlobsFilePath = "blobs-file-path"


[blocking] this flag appears unused. I think the command below should only read blobs from a file path if the optional flag is provided.

i have move the file input to a optional flag instead of argument

rootulp · 2023-12-04T18:31:37Z

x/blob/client/cli/payforblob.go

 )

 func CmdPayForBlob() *cobra.Command {
 	cmd := &cobra.Command{
-		Use: "PayForBlobs namespaceID blob",
+		Use: "PayForBlobs [namespaceID, blob] [path/to/blob.json]",


[blocking] this usage statement is a bit confusing. The [] usually describes a optional group. Here both groups are optional but that's not really the case. The command accepts one or the other.

I think we should take a step back and design the user experience.

Non-breaking approach

IMO this command should accept one namespaceID and blob. Ex.

celestia-appd tx blob PayForBlobs namespaceID blob

or multiple blobs

celestia-appd tx blob PayForBlobs --input-file path/to/blobs.json

Breaking approach

We split this into two commands (PayForBlob and PayForBlobs) and remove the aliases

celestia-appd tx blob PayForBlob namespaceID blob

celestia-appd tx blob PayForBlobs path/to/blobs.json

rootulp · 2023-12-04T18:33:46Z

x/blob/client/cli/payforblob.go

+		Example: "celestia-appd tx blob PayForBlobs path/to/blob.json \\\n" +
 			"\t--chain-id private \\\n" +
 			"\t--from validator \\\n" +
 			"\t--keyring-backend test \\\n" +
 			"\t--fees 21000utia \\\n" +
 			"\t--yes",


[blocking] if we do the non-breaking approach where this command supports both one blob and multiple blobs, then this Example section should contain both examples

$ celestia-appd tx blob PayForBlobs --help <truncated> Examples: celestia-appd tx blob PayForBlobs 0x00010203040506070809 0x48656c6c6f2c20576f726c6421 \ --chain-id private \ --from validator \ --keyring-backend test \ --fees 21000utia \ --yes celestia-appd tx blob PayForBlobs --input-file path/to/blobs.json \ --chain-id private \ --from validator \ --keyring-backend test \ --fees 21000utia \ --yes

added as suggested

rootulp · 2023-12-04T18:34:18Z

x/blob/client/cli/payforblob.go

+		Short: "Pay for a data blobs to be published to Celestia.",
+		Long: "Pay for a data blobs to be published to Celestia.\n" +


Suggested change

Short: "Pay for a data blobs to be published to Celestia.",

Long: "Pay for a data blobs to be published to Celestia.\n" +

Short: "Pay for data blob(s) to be published to Celestia.",

Long: "Pay for data blob(s) to be published to Celestia.\n" +

staheri14 · 2023-12-04T19:03:43Z

x/blob/client/cli/payforblob.go

+		Short: "Pay for a data blobs to be published to Celestia.",
+		Long: "Pay for a data blobs to be published to Celestia.\n" +
+			"User can use namespaceID and blob or path to a json file as argument" +
+			`Where blob.json contains: 


[Suggestion] To enhance the user experience, it would be beneficial to consider the implementation of flexible file naming conventions, rather than restricting to a specific file name like blob.json. This can be achieved by allowing users to include their chosen file name in the filepath, which then can be parsed accordingly.

oh yeah, i think i need to specify that blob.json is an example file name

…-app into use_file_input

evan-forbes

only one comment from the linter otherwise lgtm. thanks @bao1029p

evan-forbes · 2023-12-04T21:22:40Z

x/blob/client/cli/payforblob.go

 			if len(args) < 2 {
-				return fmt.Errorf("PayForBlobs requires two arguments: namespaceID and blob")
+				return fmt.Errorf("PayForBlobs requires two argument: namespaceID and blob")


if we're going to use fmt.Errorf, then we should include something to format in the string other wise I think the linter is going to block us

updated as request

…-app into use_file_input

cmwaters · 2023-12-05T11:35:41Z

Just as an aside, I don't know why we use camel case for PayForBlobs in the cli. No other command uses capitalized letters. It's all lower case iirc

rootulp

Fantastic work! Thanks for persisting through multiple rounds of review. I have one nit and then this LGTM

x/blob/client/cli/payforblob.go

bao1029p added 3 commits November 2, 2023 01:18

allow submit multiple blobs in CLI

d332413

add print log when fail

d66bff5

add file parse for blobs

a6c4c5e

github-actions bot added the external label Nov 19, 2023

Merge branch 'main' into use_file_input

25324ba

Merge branch 'main' into use_file_input

9f7a870

rootulp assigned bao1029p Nov 26, 2023

Merge branch 'main' into use_file_input

967e1e9

sontrinh16 and others added 3 commits November 29, 2023 15:36

debugging

488c541

add new proto for blobJSON

0be304e

Merge pull request #1 from decentrio/use_file_input

e6106ca

Use file input

bao1029p changed the title ~~Use file input~~ feat: Use file input in payForBlobs CLI to allow to submit multiple blobs Nov 29, 2023

bao1029p and others added 6 commits November 29, 2023 16:41

minor clean up

5ff6bce

minor

0be0e04

fix testcase

64366d6

pull upstream

bbc905c

minor

c978ada

Merge branch 'main' into use_file_input

4892a89

bao1029p marked this pull request as ready for review November 30, 2023 14:39

bao1029p requested review from evan-forbes, rootulp and cmwaters as code owners November 30, 2023 14:39

evan-forbes reviewed Nov 30, 2023

View reviewed changes

sontrinh16 and others added 5 commits December 4, 2023 23:11

Merge branch 'main' into use_file_input

bcccab1

resolve state breaking issue by execpt both usage of input

8b5e6c3

Merge branch 'use_file_input' of https://github.com/bao1029p/celestia…

84613f0

…-app into use_file_input

minor

607e3d7

Merge branch 'main' into use_file_input

5becf2a

rootulp reviewed Dec 4, 2023

View reviewed changes

remove unused flag

d8fccc9

staheri14 reviewed Dec 4, 2023

View reviewed changes

sontrinh16 added 2 commits December 5, 2023 02:51

switch to use flag for input file

05b3683

Merge branch 'use_file_input' of https://github.com/bao1029p/celestia…

643769d

…-app into use_file_input

celestia-bot requested a review from a team December 4, 2023 19:53

minor update

70f1dd4

evan-forbes reviewed Dec 4, 2023

View reviewed changes

Merge branch 'main' into use_file_input

37e594d

celestia-bot requested a review from a team December 4, 2023 21:33

sontrinh16 added 4 commits December 5, 2023 04:39

minor lint fix

809e289

Merge branch 'use_file_input' of https://github.com/bao1029p/celestia…

dbf7e76

…-app into use_file_input

minor command description update

332cc50

minor update for consistency

eb75bef

evan-forbes previously approved these changes Dec 4, 2023

View reviewed changes

rootulp mentioned this pull request Dec 5, 2023

Consider kebab-casing PayForBlobs in the CLI #2901

Closed

rootulp reviewed Dec 5, 2023

View reviewed changes

x/blob/client/cli/payforblob.go Outdated Show resolved Hide resolved

rootulp dismissed evan-forbes’s stale review via 5421c88 December 5, 2023 16:25

Update x/blob/client/cli/payforblob.go

5421c88

rootulp approved these changes Dec 5, 2023

View reviewed changes

celestia-bot requested a review from a team December 5, 2023 16:26

rootulp requested a review from evan-forbes December 5, 2023 16:26

evan-forbes approved these changes Dec 5, 2023

View reviewed changes

rootulp merged commit 4d08a7f into celestiaorg:main Dec 5, 2023
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Use file input in payForBlobs CLI to allow to submit multiple blobs #2856

feat: Use file input in payForBlobs CLI to allow to submit multiple blobs #2856

bao1029p commented Nov 19, 2023 •

edited

Loading

coderabbitai bot commented Nov 19, 2023 •

edited

Loading

Reviews Paused

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

bao1029p commented Nov 19, 2023

codecov-commenter commented Nov 19, 2023

sontrinh16 commented Nov 29, 2023 •

edited

Loading

bao1029p commented Nov 29, 2023

bao1029p commented Nov 29, 2023

bao1029p commented Nov 30, 2023

evan-forbes left a comment

evan-forbes Nov 30, 2023

sontrinh16 Nov 30, 2023

evan-forbes Nov 30, 2023

sontrinh16 Nov 30, 2023

rootulp Dec 4, 2023

sontrinh16 Dec 4, 2023

rootulp Dec 4, 2023

sontrinh16 Dec 4, 2023

rootulp Dec 4, 2023 •

edited

Loading

rootulp Dec 4, 2023

sontrinh16 Dec 4, 2023

rootulp Dec 4, 2023

sontrinh16 Dec 4, 2023

staheri14 Dec 4, 2023

sontrinh16 Dec 4, 2023

evan-forbes left a comment

evan-forbes Dec 4, 2023

sontrinh16 Dec 4, 2023

cmwaters commented Dec 5, 2023

rootulp left a comment

		// FlagNamespaceVersion allows the user to override the namespace version when
		// submitting a PayForBlob.

		Short: "Pay for a data blobs to be published to Celestia.",
		Long: "Pay for a data blobs to be published to Celestia.\n" +

feat: Use file input in payForBlobs CLI to allow to submit multiple blobs #2856

feat: Use file input in payForBlobs CLI to allow to submit multiple blobs #2856

Conversation

bao1029p commented Nov 19, 2023 • edited Loading

Changes made

Testing

Checklist

coderabbitai bot commented Nov 19, 2023 • edited Loading

Reviews Paused

Chat with CodeRabbit Bot (@coderabbitai)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

bao1029p commented Nov 19, 2023

codecov-commenter commented Nov 19, 2023

Codecov Report

sontrinh16 commented Nov 29, 2023 • edited Loading

bao1029p commented Nov 29, 2023

bao1029p commented Nov 29, 2023

bao1029p commented Nov 30, 2023

evan-forbes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rootulp Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

Non-breaking approach

Breaking approach

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmwaters commented Dec 5, 2023

rootulp left a comment

Choose a reason for hiding this comment

bao1029p commented Nov 19, 2023 •

edited

Loading

coderabbitai bot commented Nov 19, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Configration File (`.coderabbit.yaml`)

sontrinh16 commented Nov 29, 2023 •

edited

Loading

rootulp Dec 4, 2023 •

edited

Loading